Scripts: Fork jest-environment-puppeteer to use puppeteer-core directly #29418
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gziolo
added
[Type] Enhancement
gziolo
commented
Mar 1, 2021
| testMatch: [ '**/specs/**/*.[jt]s', '**/?(*.)spec.[jt]s' ], | ||
| testPathIgnorePatterns: [ '/node_modules/' ], | ||
| reporters: |
There was a problem hiding this comment.
We no longer use Travis so this code is not used. I also don't think that travis-fold-passes-reporter.js file is still present in the Jest preset.
gziolo
commented
Mar 1, 2021
| return require( 'puppeteer-core' ); | ||
| } | ||
| case 'firefox': | ||
| return require( 'puppeteer-firefox' ); |
There was a problem hiding this comment.
Aside:
I have just learned that https://www.npmjs.com/package/puppeteer-firefox is deprecated.
There was a problem hiding this comment.
Yeah, it's now integrated into puppeteer proper, and can be run via the --puppeteer-product=firefox arg, see e.g. #28498.
We could drop this case branch (which amounts to re-organizing -- or even dropping -- the whole getPuppeteer function).
|
Size Change: +9.8 kB (+1%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
Can you explain why we needed the Alternatively: AFAIU, the main reason for using |
As noted in the description:
The patch referenced removes the hard dependency on
Yes, that's the main reason. It's inconvenient to teach all projects to use the flag you mentioned in their setup to avoid downloading Chromium. There is no simple way to define the same flag on the |
Yes, I read that before asking my question. Maybe you meant to point me to smooth-code/jest-puppeteer#315 specifically? Upon reading that PR's diff, I see that current versions of puppeteer have an explicit |
ockham
reviewed
Mar 5, 2021
ockham
reviewed
Mar 5, 2021
There was a problem hiding this comment.
Looking good overall -- thanks a lot for tackling this!
I left one note which we should probably address prior to merging this -- otherwise I think it's ready.
In the future, we might consider trimming down our copy of jest-environment-puppeteer further, or replace it by a simpler stub, as e.g. demonstrated at https://jestjs.io/docs/en/puppeteer#custom-example-without-jest-puppeteer-preset.
Yes, we should consider both options. It would be a breaking change but we can always leave a note in the CHANGELOG how to update projects. For now, by using a copy of |
ockham
approved these changes
Mar 8, 2021
This was referenced Mar 17, 2021
This was referenced Mar 18, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
https://github.com/smooth-code/jest-puppeteer didn't get any updates in the last 15 months even though there were a few enhancements merged. The repository is still looking for maintainers. One of the merged changes would allow us to avoid using an npm alias
puppeteerforpuppeteer-core: argos-ci/jest-puppeteer#315. There are reports of issues with npm aliases used as dependencies in packages published to npm, see #21872.I did some explorations and it looks like we mostly need the latest version
jest-environment-puppeteer. I thought that since it's only 3 files, we could as well fork this package and inline it in the@wordpress/scriptspackage. This way we no longer need to use the npm alias forpuppeteerand we should be able to have better control over the e2e configuration.I included the copyright note in all ported files:
How has this been tested?
npm run test-e2estill works.Types of changes
Enhancement.
Checklist: